@matanl-starkware reviewed 24 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 139 at r1 (raw file):

    is_block_info_valid(
        args.block_info_validation.clone(),
        args.block_info.clone(),

I don't like this clone

Code quote:

.clone()

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 149 at r1 (raw file):

        args.deps.batcher.clone(),
        args.deps.state_sync_client,
        args.block_info.clone(),

Same for this clone

Code quote:

.clone()

crates/apollo_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 78 at r1 (raw file):

        BlockInfo block_info          = 1;
        ProposalFin fin               = 2;
        TransactionBatch transactions = 4;

Didn't change to 3 for a reason?

Code quote:

 4;

NODE_14_CONSENSUS_FAILURE_REPORT.md line 1 at r1 (raw file):

# Node 14 Consensus Failure - Root Cause Analysis

What is this file?

Code quote:

# Node 14 Consensus Failure - Root Cause Analysis

crates/apollo_consensus/src/single_height_consensus_test.rs line 37 at r1 (raw file):

const ROUND_1: Round = 1;

fn get_block_info_for_height(height: BlockNumber, round: Round) -> ConsensusBlockInfo {

Why don't we use block_info from the other module?

Code quote:

get_block_info_for_height

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

    if repeat_proposal {
        // Duplicate block info should be ignored.
        let ret = shc.handle_proposal(&leader_fn, block_info.clone());

Why is cloning necessary now?

Code quote:

.clone()

@asmaastarkware made 3 comments.
Reviewable status: 18 of 24 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).

crates/apollo_consensus/src/single_height_consensus_test.rs line 37 at r1 (raw file):

Done.

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

ConsensusBlockInfo does not derive Copy, are u ok with deriving it?

crates/apollo_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 78 at r1 (raw file):

no, done

@matanl-starkware reviewed 12 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: 23 of 24 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

No
I would like to avoid copying altogether.
Can't we pass a ref?

@asmaastarkware made 1 comment.
Reviewable status: 23 of 24 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

handle_proposal takes ownership of ConsensusBlockInfo because it moves it into SMRequest::StartValidateProposal, which requires ownership. If we used a reference, we'd need to clone it first, which is unnecessary since the caller already owns it and doesn't need it afterward.

@matanl-starkware made 1 comment.
Reviewable status: 22 of 24 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

IIUC, when you derive Copy, you allow the compiler to silently shallow-copy the entire struct whenever required.
So either we transfer ownership and then the Copy is not required, or we have to copy the data to allow double data existence.

@matanl-starkware reviewed 5 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).

@dafnamatsry made 1 comment.
Reviewable status: 22 of 24 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matanl-starkware).

crates/apollo_protobuf/src/protobuf/protoc_output.rs line 493 at r7 (raw file):

    #[allow(clippy::derive_partial_eq_without_eq)]
    #[derive(Clone, PartialEq, ::prost::Oneof)]
    pub enum Message {

I think keeping Init and removing the BlockInfo variant would be better. Semantically, Init better describes the purpose of this message—marking the beginning of a proposal stream. We can simply embed the required BlockInfo data inside the ProposalInit struct.

So my suggestion is:

pub mod proposal_part {
    pub enum Message {
        Init(super::ProposalInit),
        Fin(super::ProposalFin),
        Transactions(super::TransactionBatch),
    }
}

pub struct ProposalInit {
    pub height: u64,
    pub round: u32,
    pub valid_round: ::core::option::Option<u32>,
    pub proposer: ::core::option::Option<Address>,

    pub block_info: BlockInfo,
}

@matanl-starkware WDYT?